Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow locking of the seeded admin user #17273

Merged
merged 6 commits into from
Nov 26, 2024
Merged

Conversation

oliverguenther
Copy link
Member

@oliverguenther oliverguenther force-pushed the feature/disable-seed-admin branch 4 times, most recently from b546504 to 74e514c Compare November 26, 2024 08:26
Copy link
Member

@cbliard cbliard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not work as expect. When running the seeding with OPENPROJECT_SEED_ADMIN_USER_ENABLED=false, it fails with non-null violation errors.

      ActiveRecord::NotNullViolation:
        PG::NotNullViolation: ERROR:  null value in column "user_id" of relation "journals" violates not-null constraint
        DETAIL:  Failing row contains (5, Project, 5, null, , 2024-11-26 08:54:39.66285+00, 1, 2024-11-26 08:54:39.66285+00, Journal::ProjectJournal, 5, {}, ["2024-11-26 08:54:39.66285+00",)).

That's because the admin user creation is skipped, but then this user is expected to exist as it is the author of all created work packages afterward.

This is controlled by Seeder#admin_user in app/seeders/seeder.rb:82.

Apart from that, I proposed some minor changes.

app/seeders/admin_user_seeder.rb Outdated Show resolved Hide resolved
config/constants/settings/definition.rb Outdated Show resolved Hide resolved
config/constants/settings/definition.rb Outdated Show resolved Hide resolved
spec/seeders/admin_user_seeder_spec.rb Outdated Show resolved Hide resolved
@oliverguenther oliverguenther force-pushed the feature/disable-seed-admin branch from 75f042a to e0104a4 Compare November 26, 2024 09:00
@oliverguenther
Copy link
Member Author

@cbliard good catch, thanks. I'd suggest we use the system user in this case.

@oliverguenther oliverguenther changed the title Disable seeding of admin user Allow locking of the seeded admin user Nov 26, 2024
@cbliard
Copy link
Member

cbliard commented Nov 26, 2024

Now it fails with "Nothing registered with reference :openproject_admin".

I noticed that the tests I added leaked some state and made subsequent tests fail. I just fixed it.

@cbliard
Copy link
Member

cbliard commented Nov 26, 2024

And seeding can't work because the assignee is set to the system user (admin user normally) and then some queries are created with assignee filter being that system user, but they can't be saved because there is a validation rule about the assignee filter that forbids non-human assignees. :/

@cbliard
Copy link
Member

cbliard commented Nov 26, 2024

Why not implement something like MINIMAL_SEEDING=true that would skip creation of demo projects, but still create all other necessary things?

@oliverguenther oliverguenther force-pushed the feature/disable-seed-admin branch 2 times, most recently from e1ed0f9 to a48cd19 Compare November 26, 2024 12:05
@oliverguenther oliverguenther force-pushed the feature/disable-seed-admin branch from a48cd19 to 834d8c5 Compare November 26, 2024 12:05
@oliverguenther
Copy link
Member Author

@cbliard Minimal seeding could be another use case, but the demo data should remain. I've talked to openDesk, and locking the user after seeding has completed seems like a good alternative. That way, we can still use that user for demo data, but ensure it's not accessible.

Copy link
Member

@cbliard cbliard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍

@oliverguenther oliverguenther merged commit a13edc8 into dev Nov 26, 2024
12 checks passed
@oliverguenther oliverguenther deleted the feature/disable-seed-admin branch November 26, 2024 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants